-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Code Generation with Weaver - Example of Code Generation for SemConv Attribute Registry and SemConv Metrics #136
[WIP] Code Generation with Weaver - Example of Code Generation for SemConv Attribute Registry and SemConv Metrics #136
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
======================================
Coverage ? 73.4%
======================================
Files ? 44
Lines ? 2504
Branches ? 0
======================================
Hits ? 1840
Misses ? 664
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the code in weaver lgtm.
Regarding rust templates - I think those either belong in an examples directory or a separate or for otel-rust. At a minimum there should be a test on them if they live in weaver.
@jsuereth yes this PR is still a draft I will add some tests today and I will move the Rust templates into an examples directory. |
/// TRACE method. | ||
Trace, | ||
/// Any HTTP method that the instrumentation has no prior knowledge of. | ||
Other, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those should have string values matching ones defined in the spec, i.e. CONNECT
, _OTHER
- is it possible in Rust?
Also, enums are notoriously difficult in other languages for their back-compat issues (calling Enum.NewValueAddedInVNext
but in runtime you deal with vOld of the enum vlass). I assume it's not an issue in Rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are ways to create this mapping, either with a macro or directly via the implementation of the Display trait. I will add that support.
To handle the evolution of enum definitions, I used the #[non_exhaustive]
macro attribute, which indicates that a type or variant may have more fields or variants added in the future (more details here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string values for the enums are now supported. See 9483d63
crates/weaver_forge/codegen_examples/mini_registry/deprecated/network.yaml
Outdated
Show resolved
Hide resolved
…to example-codegen-semconv-rust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
crates/weaver_forge/codegen_examples/semconv_registry/http-common.yaml
Outdated
Show resolved
Hide resolved
use weaver_common::Logger; | ||
|
||
/// Return a nice summary of the error. | ||
/// Return a nice summary of the error including the chain of causes. | ||
/// Only the last error in the chain is displayed with a full stack trace. | ||
pub(crate) fn error_summary(error: minijinja::Error) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this does what I think it does, maybe it's time to move the Docker image from debug=>release?
I can start a tracker but it'd be nice to rely on release builds (~100MB vs. 400MB somehow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I don't precisely remember the exact problem you observed with the release version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there were two issues:
- The error output was truncated to the point of being useless, whereas in debug build you got line numbers and pointers at problematic templates.
- The
{{ debug(var) }}
trick for templates stopped rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, pending a windows build fix
crates/weaver_codegen_test/build.rs
Outdated
if parent.is_empty() { | ||
vec![] | ||
} else { | ||
parent.split('/').map(|s| s.to_owned()).collect::<Vec<_>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line may not be windows friendly. I think you should be able to split using Path
in some fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Path.components() is the platform independent solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well not completely...
…to example-codegen-semconv-rust
IMPORTANT NOTES:
Guidance to review this PR
The purpose of this PR is to allow various maintainers of API/SDK clients to provide feedback on Weaver in the context of code generation. The generation of Rust code for SemConv is taken as an example.
File structure, under
crates/weaver_codegen_test/
:semconv_registry/
: Contains the SemConv registry used in this example.templates/registry/rust/
: Holds the templates for this PR.templates/registry/rust/weaver.xml
: Defines the JQ expression for applying each template to the resolved registry.generated_code/
: Location where the Rust SemConv code is generated.The following command can be used to reproduce this code generation:
To the attention of API/SDK client maintainers,
Weaver uses a Jinja2-compatible engine to render the templates. The following custom filters are available:
lower_case
: Converts a string to lowercase.upper_case
: Converts a string to UPPERCASE.title_case
: Converts a string to TitleCase.pascal_case
: Converts a string to PascalCase.camel_case
: Converts a string to camelCase.snake_case
: Converts a string to snake_case.screaming_snake_case
: Converts a string to SCREAMING_SNAKE_CASE.kebab_case
: Converts a string to kebab-case.screaming_kebab_case
: Converts a string to SCREAMING-KEBAB-CASE.acronym
: Replaces acronyms in the input string with the full name defined in theacronyms
section of theweaver.yaml
configuration file.split_ids
: Splits a string by '.' creating a list of nested ids.type_mapping
: Converts a semantic convention type to a target type (see weaver.yaml sectiontype_mapping
).comment_with_prefix(prefix)
: Outputs a multiline comment with the given prefix.flatten
: Converts a List of Lists into a single list with all elements.e.g. [[a,b],[c]] => [a,b,c]
attribute_sort
: Sorts a list ofAttribute
s by requirement level, then name.metric_namespace
: Converts registry.{namespace}.{other}.{components} to {namespace}.attribute_registry_file
: Converts registry.{namespace}.{other}.{components} to attributes-registry/{namespace}.md (kebab-case namespace).attribute_registry_title
: Converts registry.{namespace}.{other}.{components} to {Namespace} (title case the namespace).attribute_registry_namespace
: Converts metric.{namespace}.{other}.{components} to {namespace}.attribute_namespace
: Converts {namespace}.{attribute_id} to {namespace}.The following custom testers are available:
stable
: Tests if anAttribute
is stable.experimental
: Tests if anAttribute
is experimental.deprecated
: Tests if anAttribute
is deprecated.To the attention of the maintainers of opentelemetry-rust
@jtescher, @TommyCpp , @cijothomas , @lalitb , @djc (and any other opentelemetry-rust maintainers/contributors), please review this PR and provide feedback on both the Weaver tool and the Rust code generated from a few Jinja2 templates that I assembled specifically for this exercise.
The generated code adheres to the recommendations defined in this PR by @lmolkova, along with a few other suggestions of my own:
#[cfg(feature = "semconv_experimental")]
#[deprecated(note="{{ attribute.deprecated }}")]
.Display
trait and expose a methodas_str
returning the string value for each variant. A_Custom
variant is also added to let the users define their own variant.Below is an example showcasing the creation of metrics using the type-safe API generated by the templates. The goal is to guide our users through the smart completion offered by most IDEs (including documentation), and also to leverage Rust's type system and compiler to guide the user and prevent any omissions, errors, and misuse.
Discussion on performance:
Status
Display
trait for every enum.as_str
for every enum.Work for future PR: Generate type-safe API for spans.